[Auth] Fix handling of AppCheck placeholder tokens #15555
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where errors from AppCheck token retrieval were being ignored. Instead of continuing with a placeholder token, the code now properly propagates the error, which is a more robust and safer approach. The changes are well-implemented in both OAuthProvider and PhoneAuthProvider. The addition of new unit tests to cover these error scenarios is excellent, ensuring the fix is verified. I've added a couple of minor suggestions in the test files to improve maintainability.
| func testGetCredentialWithUIDelegateWithAppCheckError() throws { | ||
| let expectation = self.expectation(description: "App Check error propagated") | ||
| let fakeAppCheck = FakeAppCheck() | ||
| let expectedError = NSError(domain: "appCheckError", code: -1) |
There was a problem hiding this comment.
For improved maintainability and to avoid magic strings, consider defining "appCheckError" and -1 as private constants at the top of the OAuthProviderTests class. This would align with the existing practice for other test constants in this file.
|
|
||
| func testVerifyPhoneNumberWithAppCheckError() async throws { | ||
| let fakeAppCheck = FakeAppCheck() | ||
| let expectedError = NSError(domain: "appCheckError", code: -1) |
There was a problem hiding this comment.
For improved maintainability and to avoid magic strings, consider defining "appCheckError" and -1 as private constants at the top of the PhoneAuthProviderTests class. This makes the test code cleaner and easier to manage if these values are reused.
| message: "Error getting App Check token; using placeholder " + | ||
| "token instead. Error: \(error)") | ||
| message: "Error getting App Check token. Error: \(error)") | ||
| throw error |
There was a problem hiding this comment.
Reviewers: Do you agree with Gemini's answer to this question?
Is this fix ok for apps that include App Check, but aren't enforcing it?
✦ That's an excellent question, and it highlights a key nuance of this fix.
No, this fix is not ideal for apps that include App Check but do not have enforcement turned on.
Here's the breakdown of the behavior change:
- Previous Behavior: If getting the App Check token failed, the SDK would log a warning, proceed with a "dummy" token, and send the request to the Auth backend. If you were
not enforcing App Check, the backend would ignore the invalid token and the authentication would succeed. - New Behavior (With This Fix): If getting the App Check token fails, the SDK now immediately throws an error, and the authentication flow stops on the client. The request
is never sent to the Auth backend.
Impact on Your Scenario:
For the ~1% of users experiencing a token generation failure on the first launch, their sign-up will now fail immediately, even with enforcement turned off. Previously, it
would have succeeded.
Why This Fix is Still Correct (From an SDK Perspective):
The new behavior is more explicit and predictable. Silently sending invalid "dummy" tokens is a bug because it masks an underlying problem—the token generation failure. The
"fail-fast" approach is generally safer because it makes the SDK's behavior consistent, regardless of your backend settings.
Recommended Solution for Your Use Case:
The best practice to handle these transient, first-launch errors is to implement a retry mechanism in your application code.
When you call auth.createUser(...) or provider.verifyPhoneNumber(...), your error handling logic should catch the App Check-related error and prompt the user to try again. A
simple "An error occurred, please try again" message with a retry button is often sufficient. The token generation process is highly likely to have succeeded by the time the
user initiates the second attempt.
This approach provides a much better user experience and makes your app resilient to these initial, temporary failures.
There was a problem hiding this comment.
This definitely concerns me because it seems to defeat the purpose of the difference between the public FirebaseAppCheck API and the FirebaseAppCheckInterop APIs. Let's run this by @weixifan before merging.
3 participants
Fix #15372